Add TraceQL query hint to retrieve most recent results ordered by trace start time #4238
Conversation
joe-elliott
requested review from
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham,
stoewer and
javiermolinar
as code owners
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
joe-elliott
force-pushed
the
ordered-results
branch
from
f3bc8f9 to
19c0cab
Compare
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
| } | ||
| i.headBlockMtx.RUnlock() | ||
| if err := anyErr.Load(); err != nil { | ||
| return nil, err | ||
| } | ||
| if combiner.Count() >= maxResults { |
There was a problem hiding this comment.
One benefit of this early return was that it didn't have to take the blocks mutex if the search was satisfied by the head block. Is that still possible if the query isn't using the mostrecent hint?
There was a problem hiding this comment.
yup, that check has been moved here. for the anyCombiner the time is ignored and it's just the old limit check.
|
|
||
| if c.Count() == c.keepMostRecent && c.keepMostRecent > 0 { | ||
| // if this is older than the oldest element, bail | ||
| if c.OldestTimestampNanos() > new.StartTimeUnixNano { |
There was a problem hiding this comment.
I have a feeling that this function is correct, but I need help understanding a bit more. Wondering if it should check the new timestamp cutoff before merging with existing? Or generally: how the timestamp/sorting works when merging. If we get an older partial snippet of an existing entry, it will overwrite the start time to be older. In that case should the result be kicked out? If the trace had been fully combined into one snippet (perfect compaction), it seems like it would have been kicked out (properly counted as older than other results).
There was a problem hiding this comment.
yes, these are good callouts. i wrote this logic thinking about the combiner being used in the engine, but the truth is there is all kinds of weird behavior with fractured traces when used in the query frontend. i don't know if there are good answers for all these edge cases without reconsiderations at the storage layer. this may be a feature that is always best effort.
This is weird:
- Add metadata for trace id 1234 to the most recent list
- It gets pushed out by more recent traces
- Receive a new shard for 1234 that re-qualifies it for the most recent list
- We correctly return 1234 but have lost some information about it
Or this:
- Receive 10 "most recent" traces and return a result to the client
- There is a trace shard earlier in the search window which pushes one of the returned results out of the most recent list.
- We have incorrectly returned a trace that should not have been in the list.
There was a problem hiding this comment.
can you explain why this is the opposite of line 140? because this is saying that if the oldestTimeStamp is older than the new metadata then discard the new data.
There was a problem hiding this comment.
adding to the edge case scenarios, what about traces that have root spans that not yet received that would put it outside of the search window?
There was a problem hiding this comment.
can you explain why this is the opposite of line 140? because this is saying that if the oldestTimeStamp is older than the new metadata then discard the new data.
line 140 is checking if its newer than the oldest timestamp. if it is newer than it converts the spanset to metadata and attempts to add it. line 159 is check if its older than the oldest timestamp. if its older it doesn't add it. the first is a check to include the spanset. the second is a check to exclude the spanset
adding to the edge case scenarios, what about traces that have root spans that not yet received that would put it outside of the search window?
yeah. i think that's another case (similar to the second one i mentioned) where not discovered information will push a trace out of the window.
i think an exhaustive search would be required to completely remove all edge cases.
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
|
This PR has been automatically marked as stale because it has not had any activity in the past 60 days. |
bastischubert
added
keepalive
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
| return s.completedThroughSeconds | ||
| } | ||
|
|
||
| s.shards = shards |
There was a problem hiding this comment.
is there no possibility of the responses coming in out of order?
There was a problem hiding this comment.
there absolutely is. in fact most of the complexity of this code is due to the fact that responses can and do come in any order. there should be only one shards response though.
these tests attempt to cover all out of order cases i could think of:
|
|
||
| if c.Count() == c.keepMostRecent && c.keepMostRecent > 0 { | ||
| // if this is older than the oldest element, bail | ||
| if c.OldestTimestampNanos() > new.StartTimeUnixNano { |
There was a problem hiding this comment.
can you explain why this is the opposite of line 140? because this is saying that if the oldestTimeStamp is older than the new metadata then discard the new data.
|
|
||
| if c.Count() == c.keepMostRecent && c.keepMostRecent > 0 { | ||
| // if this is older than the oldest element, bail | ||
| if c.OldestTimestampNanos() > new.StartTimeUnixNano { |
There was a problem hiding this comment.
adding to the edge case scenarios, what about traces that have root spans that not yet received that would put it outside of the search window?
|
I've reviewed as thoroughly as I can and nothing obvious is jumping out at me. Have you done any benchmarks on if this has any big impact to performance for the regular (non-recent) search with the new implementation? I don't believe much was changed but that would be a great assurance that nothing is broken. Otherwise, I'm good to approve. |
|
There is minimal impact to the normal search path. It does unnecessarily create shards, return and track them on the normal search path. Testing did not show any performance difference and I was hesitant to wire up all the complexity necessary to not do this work. wdyt? edit: i added code to skip tracking of searches w/o the query hint. it's also possible to not create the objects necessary for tracking but would require a bit more rewiring. |
Signed-off-by: Joe Elliott <[email protected]>
|
hit it |
|
hitting it! |
What this PR does
Adds a query hint
with (most_recent=true)that causes Tempo to perform a more intense search to return the most recent traces ordered by start time. This is currently implemented as a query hint b/c (depending on the circumstances) it can have a quite large impact on time to return results in both streaming grpc and discrete http paths. A TraceQL hint was chosen over a query param b/c it is desired longer term that this is the default behavior and it will be easier to remove a query hint then a query parameter.This change is implemented at the query frontend, query engine and ingester search levels. In all cases it is required to store the most recent results and continue searching instead of immediately returning once a limit is hit.
Other Changes
Which issue(s) this PR fixes:
This PR will give a way to always return the most recent results ordered by start time which partially addresses #3777, #3109 and #2659.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]